Conversation
efd6d1b to
f8c6e94
Compare
| let response_size = response.get_ref().encoded_len(); | ||
| let resp = RetrieveMessagesResponse::try_from(response.get_ref()) | ||
| .map_err(|e| tonic::Status::internal(e.to_string()))?; | ||
| Ok((resp, request_size, response_size)) |
There was a problem hiding this comment.
If we want to add metrics to the clients/servers then we should leverage middleware as it will be much cleaner and keep the business logic free from this instrumentation.
You can take a look at MystenLabs/sui-rust-sdk#237 for how middleware can be added to grpc clients and https://github.com/bmwill/sui/blob/bf4f4c2e66964ba3668baa2accbf0521c9aa0e81/crates/sui-rpc-api/src/lib.rs#L229-L243 for how middleware can be added to the server side (I think we do already do some server side metrics using this paradigm.)
There was a problem hiding this comment.
Reverted the client.rs changes. Byte counting now lives in RpcP2PChannel which is the metrics wrapper around the client, so grpc::Client stays clean.
I looked at both references. The challenge with applying CallbackLayer on the client side for our use case: Client instances are cached in CommitteeSet and shared across protocols. The middleware wouldn't know which protocol is active, so we'd lose protocol labels on the bytes metrics. We need per-protocol labels to compare communication cost across protocols.
RpcP2PChannel is created per-protocol with the label, so it's the natural place for this.
6d76148 to
da96dd7
Compare
81b0bc2 to
3b7fc5c
Compare
No description provided.